-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[match-case] Fix narrowing of class pattern with union-argument. #19473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[match-case] Fix narrowing of class pattern with union-argument. #19473
Conversation
| current_type = try_expanding_sum_type_to_union(current_type, enum_name) | ||
|
|
||
| current_type = get_proper_type(current_type) | ||
| if isinstance(current_type, UnionType) and (default == current_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default == current_type was necessary, otherwise quite a few tests break.
Adding or (default is None) and passing default=None in the recursive call also seems to break stuff. Not entirely sure what's going on there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, this only works by coincidence and is incorrect in general case.
The problem is that you try to union and restrict multiple results here. This is only reasonable when none of the ranges are upper bounds, otherwise this is unsound:
class Base: pass
class A1(Base): ...
class A2(Base): ...
class A3(Base): ...
ref: type[Base] = A1
x: Base | int
if isinstance(x, ref):
reveal_type(x) # Should be Base
else:
reveal_type(x) # Should be Base | int (e.g. `x = A2()`)
Applying the logic from this PR, you get Never | Base == Base as yes_type (correct), but then you get (Base | int) \ Base == int as no_type, which is wrong. Since this has nothing to do with default, likely this piece just happens to work on our testcases?
I can suggest only performing this expansion when not any(tr.is_upper_bound for tr in proposed_type_ranges), but maybe there's a better general solution similar to the logic below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran your test example and it gives the expected results.
$ mypy tmp.py
tmp.py:11: note: Revealed type is "tmp.Base"
tmp.py:13: note: Revealed type is "tmp.Base | builtins.int"
I believe it depends on from where this is called. conditional_types is called in a few places:
refine_identity_comparison_expressionwithout a defaultconditional_types_with_intersectionwith default, which itself is called in 7 places:TypeChecker.find_type_equals_checkwithcurrent_type=self.lookup_type(expr)anddefault=NoneTypeChecker.find_isinstance_check_helperwithcurrent_type=self.lookup_type(expr)anddefault=NoneTypeChecker.infer_issubclass_mapswithcurrent_type=vartypeanddefault=NonePatternChecker.visit_as_patternwith current_type=current_typeanddefault=current_type`PatternChecker.visit_value_patternwith current_type=current_typeanddefault=get_proper_type(typ)`PatternChecker.visit_singleton_patternwith current_type=current_typeanddefault=current_type`PatternChecker.visit_sequence_patternwith current_type=inner_typeanddefault=inner_type`PatternChecker.visit_sequence_patternwith current_type=new_tuple_typeanddefault=new_tuple_type`PatternChecker.narrow_sequence_childwith current_type=outer_typeanddefault=outer_type`PatternChecker.visist_class_patternwith current_type=current_typeanddefault=current_type`
So, we can see for the isinstance calls, default is None, so this branch is actually never taken currently, but may be taken for all the call-sites within PatternChecker except possibly PatternChecker.visit_value_pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I try to replicate your example with match-case we get these results:
class Base: pass
class A1(Base): ...
class A2(Base): ...
class A3(Base): ...
ref: type[Base] = A1
x: Base | int
match x:
case ref() as y: # E: expected type in class pattern; found "type[tmp.Base]"
reveal_type(y) # E: Statement is unreachable
case other:
reveal_type(other) # Base | int
match x:
case Base() as y:
reveal_type(y) # Base
case other:
reveal_type(other) # int
match x:
case A1() as y:
reveal_type(y) # A1
case other:
reveal_type(other) # Base | intThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sterliakov I integrated your suggestion into a separate branch #19517, and was able to relax the condition to
if (
isinstance(current_type, UnionType)
and not any(tr.is_upper_bound for tr in proposed_type_ranges)
and (default in (current_type, None))
):so that it also is applied with regular isinstance branches when default=None
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| The second element is the type it would hold if it was not the proposed type, if any. | ||
| UninhabitedType means unreachable. | ||
| None means no new information can be inferred. | ||
| If default is set it is returned instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as in #19471, but this will be conflicted anyway:)
| current_type = try_expanding_sum_type_to_union(current_type, enum_name) | ||
|
|
||
| current_type = get_proper_type(current_type) | ||
| if isinstance(current_type, UnionType) and (default == current_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, this only works by coincidence and is incorrect in general case.
The problem is that you try to union and restrict multiple results here. This is only reasonable when none of the ranges are upper bounds, otherwise this is unsound:
class Base: pass
class A1(Base): ...
class A2(Base): ...
class A3(Base): ...
ref: type[Base] = A1
x: Base | int
if isinstance(x, ref):
reveal_type(x) # Should be Base
else:
reveal_type(x) # Should be Base | int (e.g. `x = A2()`)
Applying the logic from this PR, you get Never | Base == Base as yes_type (correct), but then you get (Base | int) \ Base == int as no_type, which is wrong. Since this has nothing to do with default, likely this piece just happens to work on our testcases?
I can suggest only performing this expansion when not any(tr.is_upper_bound for tr in proposed_type_ranges), but maybe there's a better general solution similar to the logic below.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Converting to draft in favor of sister PR #19517 |
|
@sterliakov Mind taking a look at #19517 ? It's a modification of this PR based on your comment #19473 (comment) |
) Fixes #19468 Based on earlier PR #19473 - refactored the `conditional_types` function. - return `UninhabitedType(), default` when no ranges are given. This corresponds to `isinstance(x, ())`, with an empty tuple, which always returns `False` at runtime. - modified #19473 to change the proposed type, rather than directly returning. This is essential to maintain the behavior of the unit test `testIsinstanceWithOverlappingPromotionTypes` - Added special casing in `restrict_subtype_away`: if the second argument is a `TypeVar`, replace it with its upper bound (crucial to get correct result in `testNarrowSelfType`) - Allow `TypeChecker.get_isinstance_type` to return empty list (fixes `isinstance(x, ())` behavior). ## Modified tests - `testIsInstanceWithEmtpy2ndArg` now correctly infers unreachable for `isinstance(x, ())`. - `testNarrowingUnionMixins` now predicts the same results as pyright playground ## New Tests - `testMatchNarrowDownUnionUsingClassPattern` (https://mypy-play.net/?mypy=1.17.0&python=3.12&gist=e9ec514f49903022bd32a82ae1774abd)
Fixes #19468
I added factorization over
UnionTypeinside theconditional_typesfunction.Then
try_expanding_sum_type_to_unionneeded to move to the start of the function so that we expandcurrent_typebefore attempting factorizating the union type.I also reformatted the docstring for better legibility.
New Tests
testMatchNarrowDownUnionUsingClassPattern(https://mypy-play.net/?mypy=1.17.0&python=3.12&gist=e9ec514f49903022bd32a82ae1774abd)